Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

🌱 Initial Hub API Tests #268

Merged
merged 44 commits into from
May 4, 2023
Merged

🌱 Initial Hub API Tests #268

merged 44 commits into from
May 4, 2023

Conversation

aufi
Copy link
Member

@aufi aufi commented Mar 17, 2023 •
edited
Loading

First part of Hub REST API test suite.

Steps

  • the simplest CRUD for all resource types
  • simple seeds tests
  • connecting resources with refs (update tests above)
  • clarify non-trivial fixtures definition and tear up&down
  • basic integration scenarios (addons&analysis)

Related to #262

@aufi aufi changed the title [WIP] Hub API Tests 🌱 [WIP] Hub API Tests Mar 17, 2023
@aufi
Copy link
Member Author

aufi commented Mar 20, 2023 •
edited
Loading

The PR is related to: #262

aufi
aufi commented Mar 22, 2023
View reviewed changes
test/api/tag/fixtures.go Outdated Show resolved Hide resolved
@aufi aufi mentioned this pull request Mar 23, 2023
Merged
jortel
jortel reviewed Mar 23, 2023
View reviewed changes
test/api/client/client.go Outdated Show resolved Hide resolved
jortel
jortel reviewed Mar 23, 2023
View reviewed changes
test/api/client/client.go Outdated Show resolved Hide resolved
aufi
aufi commented Mar 27, 2023
View reviewed changes
addon/adapter.go Outdated Show resolved Hide resolved
aufi
aufi commented Mar 28, 2023
View reviewed changes
test/api/client/client.go Outdated Show resolved Hide resolved
@aufi
Copy link
Member Author

aufi commented Mar 28, 2023 •
edited
Loading

A sample with RichClient (not fully working) to consider, but it goes slightly away from the API interaction.
aufi@5178977

Update 2022-04-06: Added rich client method together with client.Should and Must for error checking&tests failures control: 625b777

@aufi
Copy link
Member Author

aufi commented Mar 28, 2023 •
edited
Loading

@jortel @mansam @djzager @dymurray Hey! This is a draft for Hub API tests (that might serve building blocks for integration components and QE tests), we had few discussion with Jeff already, but I'd like ask you for review and your thoughts. Thanks!

We can setup a call to discuss it if needed.

dymurray
dymurray reviewed Mar 29, 2023
View reviewed changes
Copy link
Contributor

@dymurray dymurray left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My suggestion:

Samples() function as it exists is okay, since you are returning an array of applications to be used for tests. However, it feels like we are close to table driven testing without actually doing it. I think a small tweak to each test can make it table-driven without too major of a change. You should be defining a set of tests at the beginning of each function, and if you want to pull from the samples to define an application for the test that is good.

See: https://github.com/konveyor/go-konveyor-tests/blob/main/developer/applications-inventory/applications_analyze_test.go#L14 how we define the test cases at the top in a table, and this can give you flexibility of defining test cases where you expect failures and things to go wrong rather than having to write a whole separate test function for it. What do you think?

@aufi
Copy link
Member Author

aufi commented Mar 30, 2023 •
edited
Loading

My suggestion:

Samples() function as it exists is okay, since you are returning an array of applications to be used for tests. However, it feels like we are close to table driven testing without actually doing it. I think a small tweak to each test can make it table-driven without too major of a change. You should be defining a set of tests at the beginning of each function, and if you want to pull from the samples to define an application for the test that is good.

See: https://github.com/konveyor/go-konveyor-tests/blob/main/developer/applications-inventory/applications_analyze_test.go#L14 how we define the test cases at the top in a table, and this can give you flexibility of defining test cases where you expect failures and things to go wrong rather than having to write a whole separate test function for it. What do you think?

Thanks for the comment! Actually even this PR was started with the TestCases and shouldFail true/false structure (e.g. 82d9c32 or ae4d8d0). However it turned out that it doesn't have right fit for our (mostly API multi-step) tests use-case, so there are some modifications.

(WIP - going to add examples and objectives why did it in such way&updating this PR).

@aufi aufi changed the title 🌱 [WIP] Hub API Tests 🌱 Initial Hub API Tests Apr 6, 2023
@aufi
Copy link
Member Author

aufi commented Apr 6, 2023

Added richclient methods, README and sample API integration (slow) test doing an application analysis d49f50d#diff-fcd2bf711a00447192da2d46171f15d0bb6302397b523e0ba92ac9f61bbb8ff7R17-R88.

There are still few things open like update API tests with more table-driven instead of "samples-driven" CRUD tests (together with considering use Samples map instead of array). But as an initial version of Hub tests, I think this PR is ready for review.

@aufi aufi marked this pull request as ready for review April 6, 2023 13:26
@aufi aufi requested a review from sshveta April 6, 2023 17:05
jortel
jortel reviewed Apr 10, 2023
View reviewed changes
addon/adapter.go Outdated Show resolved Hide resolved
jortel
jortel reviewed Apr 10, 2023
View reviewed changes
test/api/client/client.go Outdated Show resolved Hide resolved
jortel
jortel reviewed Apr 10, 2023
View reviewed changes
test/api/client/client.go Outdated Show resolved Hide resolved
aufi added 11 commits May 3, 2023 10:32
@aufi
Update Makefile phony
1c43b4f 
Signed-off-by: Marek Aufart <[email protected]>
@aufi
Fix Makefile .PHONY
f2c8102 
Signed-off-by: Marek Aufart <[email protected]>
@aufi
Disable test-api workflow temporary
87acce0 
Signed-off-by: Marek Aufart <[email protected]>
@aufi
Un-alias client pkg in integration test
67bc4b9 
Signed-off-by: Marek Aufart <[email protected]>
@aufi
Update application create/get/list checks, bucket get
7f71124 
Signed-off-by: Marek Aufart <[email protected]>
@aufi
Update test fixtures to map
c9e329f 
Signed-off-by: Marek Aufart <[email protected]>
@aufi
Update test samples to vars
ba7f1f5 
Signed-off-by: Marek Aufart <[email protected]>
@aufi
Start binding pkg with application RichClient
e9a4766 
Signed-off-by: Marek Aufart <[email protected]>
@aufi
Replace test/client for Application API test
ef59abc 
Signed-off-by: Marek Aufart <[email protected]>
@aufi
Update binding RichClient initialization
96f0789 
Signed-off-by: Marek Aufart <[email protected]>
@aufi
Update binding richclient application methods
a00ef5c 
Signed-off-by: Marek Aufart <[email protected]>
@aufi aufi marked this pull request as ready for review May 4, 2023 09:48
@aufi
Copy link
Member Author

aufi commented May 4, 2023

The PR was updated based on discussion from call we had last week with @dymurray @jortel @djzager @mansam and clarification from Jeff this week. The PR is up for review.

I'm about submitting follow-up PRs focusing on

  • covering most of resources in basic API test with the binding package update
  • expansion of Application analysis integration test

Once this PR gets in as a foundation for Konveyor upstream tests, I'm happy discuss other close-future tasks on tests as well as running these tests on PRs/nightly.

jortel
jortel approved these changes May 4, 2023
View reviewed changes
Copy link
Contributor

@jortel jortel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM
Thanks for all of the hard work on this!

jortel
jortel reviewed May 4, 2023
View reviewed changes
test/integration/applications-inventory/analysis/windup_basic_test.go
//
// Test application analysis
// "Basic" means that there no other dependencies than the application itself (no need prepare credentials, proxy, etc)
func TestBasicAnalysis(t *testing.T) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should not be in the hub.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I partially agree, but having this in test/integration directory (instead of test/api), expecting its move to different repo (or at least be just the integration test proving that Hub can triger addon analysis) and having more advanced analysis test in different repos looked OK-ish for me. Also it could be a helpful example for other&QE how to use the test methods from Hub.
Fot that reason I put it to this PR. If this would block this PR from getting in, I can remove it.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Created an issue related to this: #310

djzager
djzager approved these changes May 4, 2023
View reviewed changes
Copy link
Member

@djzager djzager left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Thanks for all the work on this.

@aufi
Copy link
Member Author

aufi commented May 4, 2023

Thanks for your feedback! Merging.

@aufi aufi merged commit 122ec48 into konveyor:main May 4, 2023
aufi added a commit to aufi/tackle2-hub that referenced this pull request May 5, 2023
@aufi
Add identity, proxy bindings and enable API tests
dc50825 
Follow-up on konveyor#268

Signed-off-by: Marek Aufart <[email protected]>
@aufi aufi mentioned this pull request May 5, 2023
Merged
aufi added a commit that referenced this pull request May 6, 2023
@aufi
🌱 Add identity, proxy bindings and enable API tests (#311)
cdb5514 
Adding identity and proxy methods to binding package. Enabling Hub API
test execution on PRs ```Build Hub / test-api (pull_request)``` (in
disconnected mode).

Follow-up on #268

Signed-off-by: Marek Aufart <[email protected]>
This was referenced May 8, 2023
Closed
Open
@djzager djzager mentioned this pull request May 11, 2023
Closed
aufi added a commit to aufi/tackle2-hub that referenced this pull request May 16, 2023
@aufi
Add Identity and Proxy to API tests.
a50baef 
Adding Indetity and Proxy resources to API tests. Identity provides
samples with login/password, key and settingss file.

Related to konveyor#268

Signed-off-by: Marek Aufart <[email protected]>
@aufi aufi mentioned this pull request May 16, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dymurray dymurray dymurray left review comments

@jortel jortel jortel approved these changes

@djzager djzager djzager approved these changes

@sshveta sshveta Awaiting requested review from sshveta

Assignees
No one assigned
Labels
None yet
Projects
Konveyor Community Meeting Demos
Status: Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@aufi @jortel @djzager @dymurray